[Bugfix] Fix TieredSpilloverCache stats not adding correctly when shards are closed #16560
Conversation
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
peteralfonsi
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
Signed-off-by: Peter Alfonsi <[email protected]>
|
❌ Gradle check result for c2a05d4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
...-common/src/main/java/org/opensearch/common/cache/stats/TieredSpilloverCacheStatsHolder.java
Outdated
Show resolved
Hide resolved
| this.diskCacheEnabled = diskCacheEnabled; | ||
| } | ||
|
|
||
| @Override | ||
| public void removeDimensions(List<String> dimensionValues) { |
There was a problem hiding this comment.
Can't we just reuse removeDimensions method from DefaultCacheStatsHolder? Both look exactly same.
There was a problem hiding this comment.
For the TSC we check for assert dimensionValues.size() == dimensionNames.size() - 1 rather than dimensionNames.size(), so that the base case is the node whose children are the individual tier nodes, but besides the check they're otherwise the same.
...rc/internalClusterTest/java/org/opensearch/common/cache/tier/TieredSpilloverCacheBaseIT.java
Outdated
Show resolved
Hide resolved
This reverts commit 3b15a7a. Signed-off-by: Peter Alfonsi <[email protected]>
peteralfonsi
force-pushed
the
tsc-stats-bug
branch
from
381ee19
to
39f5f82
Compare
Signed-off-by: Peter Alfonsi <[email protected]>
peteralfonsi
force-pushed
the
tsc-stats-bug
branch
from
39f5f82
to
f85b719
Compare
|
❌ Gradle check result for f85b719: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for a39d586: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Peter Alfonsi <[email protected]>
peteralfonsi
force-pushed
the
tsc-stats-bug
branch
from
a39d586
to
165850e
Compare
modules/cache-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCache.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/common/cache/stats/DefaultCacheStatsHolder.java
Outdated
Show resolved
Hide resolved
...e-common/src/main/java/org/opensearch/cache/common/tier/TieredSpilloverCacheStatsHolder.java
Show resolved
Hide resolved
|
❌ Gradle check result for 165850e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Peter Alfonsi <[email protected]>
Signed-off-by: Peter Alfonsi <[email protected]>
|
❕ Gradle check result for dbd0947: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16560 +/- ##
============================================
- Coverage 72.00% 71.96% -0.04%
+ Complexity 65038 64990 -48
============================================
Files 5313 5314 +1
Lines 303454 303539 +85
Branches 43910 43919 +9
============================================
- Hits 218510 218451 -59
- Misses 67040 67149 +109
- Partials 17904 17939 +35 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Peter Alfonsi <[email protected]>
|
❌ Gradle check result for 0de6284: Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Description
Fixes a bug where the total stats for the TieredSpilloverCache are decremented incorrectly when shards were closed. Misses and evictions from both the heap and disk tier were subtracted from the total, but this is incorrect. When the disk tier is enabled, only disk-tier misses and evictions should count towards the cache total, so only they should be subtracted. Adds UTs and ITs around this.
Also adds UT coverage for TieredSpilloverCacheStatsHolder, which was missing before.
Related Issues
Resolves #16559
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.